Should we just parametrize here and have a test for validation fails and validation successes?

(same questions below)

Otherwise, tests lgtm

NB both of these are potentially inefficient; .keys() creates a new PyList of all the keys.

yup, I don't love it, but at least it's only called when allow_partial=true.

Is there a more efficient way? (especially for dicts?)

Long term, if we move to RustModel and iterating over dicts when building typeddicts, we should be able to minimise use of this.

.call_method0("keys").iter().last() might be better (Python keys iterator rather than list), but I think it still sucks. Can't get better until we iterate, as you say.

I tested by adding:

#[pyfunction]
pub fn mapping_last_key_a<'a>(mapping: &'a Bound<'a, PyMapping>) -> Option<Bound<'a, PyAny>> {
    mapping.keys().ok()?.iter().ok()?.last()?.ok()
}

#[pyfunction]
pub fn mapping_last_key_b<'a>(mapping: &'a Bound<'a, PyMapping>) -> Option<Bound<'a, PyAny>> {
    mapping.call_method0(intern!(mapping.py(), "keys")).ok()?.iter().ok()?.last()?.ok()
}

and tested with

from typing import Mapping
import timeit

from pydantic_core import _pydantic_core


class MyMapping(Mapping):
    def __init__(self, d):
        self._d = d

    def __getitem__(self, key):
        return self._d[key]

    def __iter__(self):
        return iter(self._d)

    def __len__(self):
        return len(self._d)


mapping = MyMapping({str(i): i for i in range(100)})

v = _pydantic_core.mapping_last_key_a(mapping)
assert v == '99', v
v = _pydantic_core.mapping_last_key_b(mapping)
assert v == '99', v


def run_bench(func):
    timer = timeit.Timer(
        "func(mapping)", setup="", globals={"func": func, "mapping": mapping}
    )
    n, t = timer.autorange()
    iter_time = t / n
    # print(f'{func.__module__}.{func.__name__}', iter_time)
    return int(iter_time * 1_000_000_000)


print(f'mapping_last_key_a: {run_bench(_pydantic_core.mapping_last_key_a)}ns')
print(f'mapping_last_key_b: {run_bench(_pydantic_core.mapping_last_key_b)}ns')

Output:

mapping_last_key_a: 2487ns
mapping_last_key_b: 1918ns

Outcome: they're both pretty slow, but your suggestion is slightly faster.

Rather than forcing dynamic dispatch here, it might be better to use .peekable() at the callsites to be able to check if the current iteration was the last as part of the error pathway.

great idea!

peekable made things worse, but I got rid of the Box dyn in 47f1c15

and it helped quite a lot locally - comparing main to 47f1c15:

┏━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┓
┃  Group      ┃  Benchmark                          ┃  Before (µs/iter)  ┃  After (µs/iter)  ┃  Change  ┃
┡━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━┩
│  List       │  list_of_ints_core_py               │             29.17  │            30.09  │  +3.17%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  List JSON  │  list_of_ints_core_json             │             42.49  │            43.57  │  +2.55%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  Set        │  set_of_ints_core                   │             52.26  │            52.81  │  +1.06%  │
│             │  set_of_ints_core_duplicates        │             35.59  │            35.51  │  -0.22%  │
│             │  set_of_ints_core_length            │             53.59  │            55.16  │  +2.94%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  Set JSON   │  set_of_ints_core_json              │             56.31  │            58.40  │  +3.70%  │
│             │  set_of_ints_core_json_duplicates   │             44.14  │            45.07  │  +2.12%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  FrozenSet  │  frozenset_of_ints_core             │             16.60  │            17.26  │  +3.99%  │
│             │  frozenset_of_ints_duplicates_core  │             13.78  │            14.78  │  +7.20%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  Dict       │  dict_of_ints_core                  │             85.74  │            86.36  │  +0.73%  │
├─────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼──────────┤
│  Dict JSON  │  dict_of_ints_core_json             │             127.4  │            126.5  │  -0.72%  │
└─────────────┴─────────────────────────────────────┴────────────────────┴───────────────────┴──────────┘

Why not? At least for variadic tuples this seems potentially acceptable.

I definitely want to support it in future, I was just implementing it for the minimum set of validators to prove the idea initially.

Why is the .supports_partial() guard necessary (here and in general)?

because if you enabled allow_partial for a validator which doesn't have proper support for the feature, you would end up skipping errors in the wrong places.

E.g. if you had list[tuple[list[int], list[str]]:

• the outer list validator supports allow_partial, so it'll ignore errors in the last entry in the input list
• but if it passed allow_partial=true down to the tuple validator (which doesn't current support) allow_partial
• it would pass state through to both inner list validators unchanged
• then the list validator associated with the 0th entry in the tuple would get allow_partial=true
• and therefore errors in the last entry of the 0th member of the tuple in the last entry of the outer list would be ignored when they shouldn't be

It feels to me like the "doesn't support partial" is the exception rather than the rule, and probably only for non-variadic tuples? Otherwise, I would have thought that:

• collections would always want to respect allow_partial when passed in (forwarding it to their last element)
• everything else e.g. with_default etc should not even care about allow_partial and just forward it naively

Ok I'm feeling very baited to refactor these error combiners now 😂

Opened #1517 as a spike to start down that road.

.call_method0("keys").iter().last() might be better (Python keys iterator rather than list), but I think it still sucks. Can't get better until we iterate, as you say.

It feels to me like the "doesn't support partial" is the exception rather than the rule, and probably only for non-variadic tuples? Otherwise, I would have thought that:

• collections would always want to respect allow_partial when passed in (forwarding it to their last element)
• everything else e.g. with_default etc should not even care about allow_partial and just forward it naively

It feels like there's very common use cases missing from here:

• unions
• nullable
• defaults

I think validators fit into three cases:

• already done
• doesn't apply - things like IntValidator where allow_partial doesn't mean anything
• TODO simple cases - things like nullable where AFAIK it's just a matter of passing the allow_partial instruction to its nested validator - that's my understanding of these but maybe it's more complicated
• TODO - collections or multiple nested validators where we need custom logic

I think validators break down like this:

If that table is correct, we should set the default for supports_partial to true, then set it to false specifically for fields which don't support it. But I'm still not that confident I'm write about all those cases.

Done.

I got rid of supports_partial completely, and instead set state.allow_partial = false on the few validators which don't support it yet.